[DPE-2119] fix issues and tests when reusing storage #272

reneradoi · 2024-04-30T09:24:26Z

Issue

When attaching an existing storage to a new unit, 2 issues happen:

Snap install failed because of permissions / ownership of directories
snap_common gets completely deleted

Solution

bump snap version, use the fixed one (the fixed revision is 47, this is already outdated as a newer version of the snap is already available and merged to main prior to this PR)
enhance test coverage for integration tests

Integration Testing

Tests for attaching existing storage can be found in integration/ha/test_storage.py. There are now three test cases:

test_storage_reuse_after_scale_down: remove one unit from the deployment, afterwards add a new one re-using the storage from the removed unit. check if the continuous writes are ok and a testfile that was created intially is still there.
test_storage_reuse_after_scale_to_zero: remove both units from the deployment, keep the application, add two new units using the storage again. check the continuous writes.
test_storage_reuse_in_new_cluster_after_app_removal: from a cluster of three units, remove all of them and remove the application. deploy a new application (with one unit) to the same model, attach the storage, then add two more units with the other storage volumes. check the continuous writes.

Other Issues

As part of this PR, another issue is addressed: #306. It is resolved with this commit: 19f843c

…hen reusing de-attached storage)

…ge instead of rootfs

…wn to 1

…down not all shards are assigned

…ter_app_removal` as it currently does not work

reneradoi · 2024-04-30T15:16:35Z

This PR updates the installed snap to the fixed version which can install on existing storage and does not overwrite the data in snap_common.

Other content of this PR:

The integration test test_storage_reuse_after_scale_down is now working. I've:

created a storage pool on lxd
deployed the application with storage from that pool
adjusted the test workflow a little to make it more robust
re-attaching storage to a new unit tests correct now

The test case test_storage_reuse_in_new_cluster_after_app_removal is being skipped. Currently this test fails all the time, I first need to understand how this is supposed to work before fixing this test.

Mehdi-Bendriss

Thanks René! Could we add a test which does:

stopping continuous writes
scale-down to 0 units (while keeping storage around)
scale-up to N units with storage attach
check if the continuous writes data has been indeed been kept and is usable

tests/integration/ha/test_storage.py

… this ensures enough data gets written by then

…can still be run in case of hooks failure

… in re-attached storage is persistent

…nd scaling up again with re-attached storage as this currently does not work in general

phvalguima

Looks good! Let's give this logic a try with the integration tests and see how it goes.

tests/integration/ha/test_storage.py

…al, adjust the logic to destroy the application due to #243

…uster with re-attached storage

…hen reusing de-attached storage)

…ge instead of rootfs

…application

tests/integration/ha/test_storage.py

…rm_node_lock index is not available

…index (will be tracked in #321)

- block until removed test_storage.py: remove the application completely, blocking until it's removed; use wait_until from helpers instead of from ops_test to be more secure on application availability

…curity index (is already present in main)

…-attach-existing-storage

phvalguima

Hey @reneradoi I had a similar issue as yours, where there were units initially available for shards but they were still hanging as "UNASSIGNED". Although that was not about the lock itself, I think we can troubleshoot your problem here the same way.

I've added this logic here to give me more logs. You have to tweak the "explain" API to target the charm lock itself.

lib/charms/opensearch/v0/constants_charm.py

lib/charms/opensearch/v0/opensearch_locking.py

…llback to peer databag lock if it's 2 or less units

phvalguima

LGTM. My only remark is that we need to dig more into what is causing the 503.

On a separate PR, we should log 503 errors and its explanation, as follows:

GET _cluster/allocation/explain
{
  "index": ".charm-node-lock",
}

reneradoi · 2024-06-10T18:50:41Z

LGTM. My only remark is that we need to dig more into what is causing the 503.

On a separate PR, we should log 503 errors and its explanation, as follows:
GET _cluster/allocation/explain
{
  "index": ".charm-node-lock",
}

Yes that's true, I'll add another issue or draft PR, will think about it.

reneradoi added 11 commits April 30, 2024 09:19

update revision number of installed snap (fixes installation issue w…

1227daf

…hen reusing de-attached storage)

test_storage.py: add storage pool, deploy model with persistent stora…

e149fbb

…ge instead of rootfs

test_storage.py: adjust testing workflow, deploy 2 units and scale do…

069441a

…wn to 1

test_storage.py: app status will not be active because after scaling …

ea43e97

…down not all shards are assigned

test_storage.py: force-destroy the application when removing the cluster

5ad2aa8

test_storage.py: fix comment

f82d403

test_storage.py: formatting

58d6e36

test_storage.py: make test execution more robust

ecb86b0

test_storage.py: formatting

2950429

test_storage.py: use destroy_unit to scale down

d413e30

test_storage.py: skip test case `test_storage_reuse_in_new_cluster_af…

64089a9

…ter_app_removal` as it currently does not work

reneradoi marked this pull request as ready for review April 30, 2024 15:11

reneradoi requested review from Mehdi-Bendriss and phvalguima April 30, 2024 15:16

Mehdi-Bendriss reviewed Apr 30, 2024

View reviewed changes

tests/integration/ha/test_storage.py Show resolved Hide resolved

tests/integration/ha/test_storage.py Show resolved Hide resolved

tests/integration/ha/test_storage.py Show resolved Hide resolved

tests/integration/ha/test_storage.py Outdated Show resolved Hide resolved

reneradoi added 8 commits May 2, 2024 08:30

test_storage.py: get the continuous writes result after the scale-up,…

209620e

… this ensures enough data gets written by then

test_storage.py: force unit removal when scaling down to ensure test …

eb4285a

…can still be run in case of hooks failure

test_storage.py: create testfile before scaling down to check if data…

c708137

… in re-attached storage is persistent

test_storage.py: add test_storage_reuse_after_scale_to_zero

fd953cc

test_storage.py: remove skip-mark

ea7c596

test_storage.py: linting result

8b70fbe

test_storage.py: skip the newly added test for scaling down to zero a…

6b5d695

…nd scaling up again with re-attached storage as this currently does not work in general

test_storage.py: linting result

ee9e8c8

phvalguima requested changes May 5, 2024

View reviewed changes

tests/integration/ha/test_storage.py Show resolved Hide resolved

tests/integration/ha/test_storage.py Outdated Show resolved Hide resolved

tests/integration/ha/test_storage.py Show resolved Hide resolved

tests/integration/ha/test_storage.py Outdated Show resolved Hide resolved

reneradoi added 5 commits May 6, 2024 08:06

test_storage.py: continue writing data to check opensearch availability

3de3a61

test_storage.py: in test_storage_reuse_in_new_cluster_after_app_remov…

55b8ec4

…al, adjust the logic to destroy the application due to #243

test_storage.py: restart continuous writes after deployment of new cl…

83d92e3

…uster with re-attached storage

update revision number of installed snap (fixes installation issue w…

af6f769

…hen reusing de-attached storage)

test_storage.py: add storage pool, deploy model with persistent stora…

3e19d4d

…ge instead of rootfs

test_storage.py: more need to scale down carefully when removing the …

fb850c2

…application

phvalguima requested changes Jun 3, 2024

View reviewed changes

tests/integration/ha/test_storage.py Outdated Show resolved Hide resolved

tests/integration/ha/test_storage.py Show resolved Hide resolved

reneradoi added 3 commits June 3, 2024 07:23

fix copyright date

e4ad40e

restart and assert continuous writes after scale down to zero

0420245

restart and assert continuous writes after cluster removal too

b716b37

reneradoi requested a review from phvalguima June 3, 2024 12:51

reneradoi added 10 commits June 4, 2024 06:00

don't block the model when removing the application

12486d6

test_storage.py: adjust workflow with app removal

ebb7da0

add workaround for locking mechanism to avoid deadlocks when the .cha…

db41ade

…rm_node_lock index is not available

temporary fix to avoid timeout on the initialization of the security …

c48f6bb

…index (will be tracked in #321)

- wait until

4ab6b09

- block until removed test_storage.py: remove the application completely, blocking until it's removed; use wait_until from helpers instead of from ops_test to be more secure on application availability

another case where we need to fallback to peer databag lock

afde28f

assert the continuous writes differently

6820215

remove temporary fix to avoid timeout on the initialization of the se…

f1d6142

…curity index (is already present in main)

Merge remote-tracking branch 'refs/remotes/origin/main' into DPE-2119…

7988c16

…-attach-existing-storage

revert to snap revision 50 (opensearch 2.13.0)

bb74991

phvalguima requested changes Jun 10, 2024

View reviewed changes

lib/charms/opensearch/v0/constants_charm.py Outdated Show resolved Hide resolved

lib/charms/opensearch/v0/opensearch_locking.py Show resolved Hide resolved

lib/charms/opensearch/v0/opensearch_locking.py Outdated Show resolved Hide resolved

reneradoi added 2 commits June 10, 2024 14:21

use snap revision 51 again (opensearch 2.14.0)

cb15cc3

in case of error checking which unit has the OpenSearch lock, only fa…

269a4fa

…llback to peer databag lock if it's 2 or less units

reneradoi requested a review from phvalguima June 10, 2024 14:35

phvalguima previously approved these changes Jun 10, 2024

View reviewed changes

only fallback to peer databag lock if it's 1 units remaining

9e75671

reneradoi dismissed phvalguima’s stale review via 9e75671 June 11, 2024 06:29

Mehdi-Bendriss approved these changes Jun 11, 2024

View reviewed changes

phvalguima approved these changes Jun 11, 2024

View reviewed changes

reneradoi merged commit 070182a into main Jun 11, 2024
21 of 28 checks passed

reneradoi deleted the DPE-2119-attach-existing-storage branch June 11, 2024 12:57

This was referenced Jun 11, 2024

hook "leader-elected" fails when adding a unit after scale down to zero units #306

Closed

Destroying opensearch application results in 2/3 errored units #243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-2119] fix issues and tests when reusing storage #272

[DPE-2119] fix issues and tests when reusing storage #272

reneradoi commented Apr 30, 2024 •

edited

Loading

reneradoi commented Apr 30, 2024 •

edited

Loading

Mehdi-Bendriss left a comment

phvalguima left a comment

phvalguima left a comment

phvalguima left a comment

reneradoi commented Jun 10, 2024

[DPE-2119] fix issues and tests when reusing storage #272

[DPE-2119] fix issues and tests when reusing storage #272

Conversation

reneradoi commented Apr 30, 2024 • edited Loading

Issue

Solution

Integration Testing

Other Issues

reneradoi commented Apr 30, 2024 • edited Loading

Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

phvalguima left a comment

Choose a reason for hiding this comment

phvalguima left a comment

Choose a reason for hiding this comment

phvalguima left a comment

Choose a reason for hiding this comment

reneradoi commented Jun 10, 2024

reneradoi commented Apr 30, 2024 •

edited

Loading

reneradoi commented Apr 30, 2024 •

edited

Loading